Skip to content

Interpolate DynamicPPL.check_dot_tilde_rhs into macro #867

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 28, 2025
Merged

Interpolate DynamicPPL.check_dot_tilde_rhs into macro #867

merged 2 commits into from
Mar 28, 2025

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Mar 27, 2025 •
edited
Loading

Fixes #868

The function itself needs to be interpolated, otherwise it will be left to the caller to resolve. This is OK if they have imported DynamicPPL (as used to be the case in Turing 0.36) but now since Turing no longer exports DynamicPPL, check_dot_tilde_rhs can no longer be accessed by the user.

I suppose this also guards against users redefining the function themselves 😄

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 27, 2025 •
edited
Loading

Benchmark Report for Commit 8fdf764

Computer Information

Julia Version 1.11.4
Commit 8561cc3d68d (2025-03-10 11:36 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results

|                 Model | Dimension |  AD Backend |      VarInfo Type | Linked | Eval Time / Ref Time | AD Time / Eval Time |
|-----------------------|-----------|-------------|-------------------|--------|----------------------|---------------------|
| Simple assume observe |         1 | forwarddiff |             typed |  false |                  9.3 |                 1.7 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                630.9 |                44.8 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                432.7 |                47.6 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1236.7 |                28.7 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               4230.3 |                19.1 |
|           Smorgasbord |       201 | reversediff |             typed |   true |               1472.8 |                29.3 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |                967.4 |                 5.4 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               5607.9 |                 4.0 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |               1164.0 |                 8.1 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              62358.6 |                 3.9 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               9396.4 |                 9.4 |
|               Dynamic |        10 |    mooncake |             typed |   true |                131.1 |                13.5 |
|              Submodel |         1 |    mooncake |             typed |   true |                 25.6 |                 7.3 |
|                   LDA |        12 | reversediff |             typed |   true |                482.2 |                 6.0 |

@codecov Codecov
Copy link

codecov bot commented Mar 27, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.87%. Comparing base (fb5413f) to head (8fdf764).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #867   +/-   ##
=======================================
  Coverage   84.87%   84.87%           
=======================================
  Files          34       34           
  Lines        3815     3815           
=======================================
  Hits         3238     3238           
  Misses        577      577

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coveralls
Copy link

coveralls commented Mar 27, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 14113498477

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-3.2%) to 81.718%
Files with Coverage Reduction New Missed Lines %
src/varinfo.jl 3 83.83%
Totals Coverage Status
Change from base Build 14024259529: -3.2%
Covered Lines: 3111
Relevant Lines: 3807
💛 - Coveralls

@coveralls
Copy link

coveralls commented Mar 27, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 14114662600

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 84.965%
Files with Coverage Reduction New Missed Lines %
src/varinfo.jl 3 84.51%
Totals Coverage Status
Change from base Build 14024259529: 0.0%
Covered Lines: 3238
Relevant Lines: 3811
💛 - Coveralls

@devmotion
Copy link
Member

IIRC we thought about this a long time ago - I'm still not sure whether it's (easily) possible but it would have prevented this issue: Put esc only where necessary, and let otherwise handle Julia the macro hygiene (it would have referred to the function in DynamicPPL automatically here). Generally, when it's possible, it's IMO the easier and safer approach.

@penelopeysm
Copy link
Member Author

Totally agree, but I remember looking into it once and being scared away by #308 and #311.

Maybe we could reopen #308 so that someone could take a stab at it if they felt inclined to.

@penelopeysm penelopeysm force-pushed the py/fix-dot-tilde-macro branch from 0e2fac1 to 8fdf764 Compare March 27, 2025 19:02
@penelopeysm
Copy link
Member Author

@devmotion Since you commented, I'll ask you to green-light this :)

@penelopeysm penelopeysm requested a review from devmotion March 27, 2025 23:09
@penelopeysm penelopeysm added this pull request to the merge queue Mar 28, 2025
Merged via the queue into main with commit eed80e5 Mar 28, 2025
34 checks passed
@penelopeysm penelopeysm deleted the py/fix-dot-tilde-macro branch March 28, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devmotion devmotion devmotion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DynamicPPL not defined in Main error when sampling from model with .~ expression
3 participants
@penelopeysm @coveralls @devmotion